-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor open preview logic #870
base: main
Are you sure you want to change the base?
Refactor open preview logic #870
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we shouldn't update package.json as well?
als left an inline question
await commands.executeCommand("setContext", "RNIDE.sidePanelIsClosed", false); | ||
|
||
const panelLocation = workspace | ||
.getConfiguration("RadonIDE") | ||
.get<PanelLocation>("panelLocation"); | ||
|
||
if (panelLocation !== "tab") { | ||
SidePanelViewProvider.showView(context, fileName, lineNumber); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is context not necessery in Side panel while it remains necessary in tabpanel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
showView
doesn't use it at all, so I removed it 💁♂️
From what I quickly went through, SidePanelViewProvider gets context in the constructor and, being a static method, fails gracefully when it isn't provided. I didn't go deeper, as I assume this is outside the scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #278
This PR refactors logic related to opening previews. We were using
showIDEPanel
forRNIDE.showPanel
/RNIDE.openPanel
commands, the launch preview depended on the existence of thefileName
andlineNumber
. The problem is that VSCode passes its arguments there, but instead number in lineNumber we get an object with the panel's id, which triggers this logic always. Thus, I refactored the opening preview to use a separate command.Additionally, startPreview was running even if the project wasn't set up yet, which led to an error that we used
.project
onnull
. Now, we open the panel and show a warning in that case.How Has This Been Tested:
Repeat those scenarios for preview both in
tab panel
andside panel
:Steps:
Expected:
The panel opens without error dialog.
Steps:
Expected:
Confirm that panel opens and there is warning message
Steps:
Expected:
Confirm that preview started
Tested on react-native-76